Fix homepage layout alignment and responsiveness#993
Fix homepage layout alignment and responsiveness#993Prakash1185 wants to merge 3 commits intoapache:masterfrom
Conversation
|
Hi @zhongjiajie, @SbloodyS |
src/components/Footer/index.scss
Outdated
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Please leave an blank line at the end of each file.
There was a problem hiding this comment.
Pull request overview
This PR significantly improves the responsive design and layout of the homepage by introducing comprehensive media queries and CSS improvements across all homepage sections. The changes address multiple layout and alignment issues on mobile, tablet, and desktop screens.
Changes:
- Added responsive breakpoints (1280px, 1024px, 768px, 640px, 480px) throughout homepage components with appropriate sizing and spacing adjustments
- Converted fixed dimensions to flexible, responsive values (min-height, percentages, auto)
- Changed images from hidden on mobile to responsive scaling for better content visibility
- Enhanced button layouts with proper wrapping and full-width behavior on small screens
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/views/Home/why.scss | Added responsive padding, flexible image sizing, improved title wrapping, and button spacing on mobile |
| src/views/Home/tasks.scss | Changed to min-height, improved title alignment, fixed task item sizing, refined animations |
| src/views/Home/index.scss | Enhanced hero section with responsive carousel, flexible text sizing, improved button layouts, and GitHub button responsiveness |
| src/views/Home/features.scss | Added responsive feature card sizing with proper centering on smaller screens |
| src/views/Home/events.scss | Converted to flexible layouts, responsive images, and improved card padding across breakpoints |
| src/views/Home/community.scss | Made GitHub stats and community image responsive, improved layout stacking on mobile |
| src/views/Home/Main.jsx | Added wrap prop to Space component and wrapped buttons div with responsive class |
| src/views/Home/Events.jsx | Removed fixed image dimensions, added responsive sizing with width 100% and height auto |
| src/components/Footer/index.scss | Changed to min-height, added responsive link layouts, improved visibility of contact info on mobile |
| src/components/Connect/index.scss | Enhanced "Stay Connect" section with responsive text sizing and button adjustments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| line-height: 1; | ||
| position: relative; | ||
| top: -30px; | ||
| top: 0; |
There was a problem hiding this comment.
The top: 0 property effectively resets the original -30px positioning. While this improves alignment on larger screens, verify that the visual positioning of the task count number relative to surrounding text is acceptable across all breakpoints. The original negative top value was likely intentional for visual alignment.
| animation: 30s linear infinite slidein; | ||
| } | ||
|
|
||
| &-left { | ||
| animation: 30s linear 0s infinite running slideout; | ||
| animation: 30s linear infinite slideout; |
There was a problem hiding this comment.
The animation timing has been changed from 30s linear 0s infinite running slidein to 30s linear infinite slidein. While both are functionally equivalent (the 0s delay and running state are defaults), ensure this simplified syntax is consistent with project conventions and doesn't introduce any unexpected behavior.
| margin-top: 42px; | ||
| } | ||
| .github-button { | ||
| padding: 0 20px; |
There was a problem hiding this comment.
The content-width mixin already applies padding: 0 2vw for screens under 1280px. Adding padding: 0 20px here will override that padding, resulting in fixed 20px padding instead of the responsive 2vw padding. On a 1280px screen, 2vw equals 25.6px, which is close to 20px. However, on smaller screens like 640px, 2vw equals 12.8px, but this code forces 20px, which may be too much padding on mobile devices. Consider using the mixin's existing responsive padding or adjusting it conditionally with media queries.
| padding: 0 20px; | |
| @media screen and (min-width: 1281px) { | |
| padding: 0 20px; | |
| } |
src/views/Home/index.scss
Outdated
| padding: 0 20px; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
The content-width mixin already applies padding: 0 2vw for screens under 1280px. Adding padding: 0 20px here will override that padding, resulting in fixed 20px padding instead of the responsive 2vw padding. On smaller screens like 640px, 2vw equals 12.8px, but this code forces 20px, which may be excessive. Consider using the mixin's existing responsive padding or adjusting it conditionally with media queries.
src/components/Footer/index.scss
Outdated
| height: auto; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| justify-content: center; |
There was a problem hiding this comment.
The footer links are centered with justify-content: center on all screen sizes, but the original design likely had them spread across the footer width with justify-content: space-between. This changes the visual layout significantly. If centering is intentional for better mobile responsiveness, consider keeping space-between on larger screens and only switching to center on smaller screens where wrapping occurs.
| padding: 0 20px; | ||
| box-sizing: border-box; | ||
|
|
There was a problem hiding this comment.
The content-width mixin already applies padding: 0 2vw for screens under 1280px. Adding padding: 0 20px here will override that padding, resulting in fixed 20px padding instead of the responsive 2vw padding. On smaller screens like 640px, 2vw equals 12.8px, but this code forces 20px, which may be excessive. Consider using the mixin's existing responsive padding or adjusting it conditionally with media queries.
| padding: 0 20px; | |
| box-sizing: border-box; | |
| box-sizing: border-box; | |
| @media screen and (min-width: 1280px) { | |
| padding: 0 20px; | |
| } |
| margin-top: -100px; | ||
| position: relative; | ||
| z-index: 3; | ||
| padding: 0 20px; |
There was a problem hiding this comment.
The content-width mixin already applies padding: 0 2vw for screens under 1280px. Adding padding: 0 20px here will override that padding, resulting in fixed 20px padding instead of the responsive 2vw padding. On smaller screens like 640px, 2vw equals 12.8px, but this code forces 20px, which may be excessive. Consider using the mixin's existing responsive padding or adjusting it conditionally with media queries.
| padding: 0 20px; |
src/components/Connect/index.scss
Outdated
|
|
||
|
|
There was a problem hiding this comment.
There's a trailing blank line in this CSS block. While not functionally problematic, it's inconsistent with the rest of the codebase's formatting. Consider removing the blank line for consistency.
| position: absolute; | ||
| bottom: 46px; | ||
| right: 46px; |
There was a problem hiding this comment.
The button positioning changes from absolute to static based on screen size, which is good for responsive design. However, ensure that on screens >= 1025px the absolute positioning doesn't cause the button to overlap with content when the card content is shorter than expected. Consider testing with varying content lengths.
| position: absolute; | |
| bottom: 46px; | |
| right: 46px; | |
| position: static; | |
| margin-top: 24px; |
| .ant-image { | ||
| max-width: 50%; | ||
|
|
||
| @media screen and (max-width: 1024px) { | ||
| max-width: 100%; | ||
| width: 100%; | ||
| } | ||
|
|
||
| img { | ||
| width: 100%; | ||
| height: auto; | ||
| } | ||
| } |
There was a problem hiding this comment.
The change from hiding the image (display: none) to showing it responsively (max-width: 100%) is an improvement for smaller screens. However, verify that this doesn't cause performance issues on mobile devices, especially if the images are large. Consider implementing lazy loading or using responsive image sources (srcset) if the images are particularly large.
|
Thanks for the review, @SbloodyS. |
Summary
Problem
Changes Made
📸 Screenshots ( Desktop , Mobile , Tablet )
Before vs After
Herosection
Tasks
Why DolphinScheduler
Features
Community
Latest Posts
Stay connect
Footer